Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make extract_levels preprocessor function lazy #1761

Merged
merged 9 commits into from
May 30, 2023
Merged

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Oct 19, 2022

Description

Combined with v0.3 of stratify this makes the extract_levels preprocessor function lazy.

Related to #674 and #35.


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@bouweandela bouweandela changed the title Make extract_levels lazy Make extract_levels preprocessor function lazy Oct 19, 2022
@bouweandela bouweandela added the preprocessor Related to the preprocessor label Oct 19, 2022
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #1761 (efbb7e3) into main (83b12a2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1761   +/-   ##
=======================================
  Coverage   92.79%   92.80%           
=======================================
  Files         236      236           
  Lines       12485    12490    +5     
=======================================
+ Hits        11586    11591    +5     
  Misses        899      899           
Impacted Files Coverage Δ
esmvalcore/preprocessor/_other.py 100.00% <100.00%> (ø)
esmvalcore/preprocessor/_regrid.py 98.13% <100.00%> (-0.01%) ⬇️

@bouweandela bouweandela marked this pull request as ready for review May 9, 2023 20:11
@bouweandela bouweandela added this to the v2.9.0 milestone May 9, 2023
Comment on lines +77 to +87
def get_array_module(*args):
"""Return the best matching array module.

If at least one of the arguments is a :class:`dask.array.Array` object,
the :mod:`dask.array` module is returned. In all other cases the
:mod:`numpy` module is returned.
"""
for arg in args:
if isinstance(arg, da.Array):
return da
return np
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function shouldn't be needed thanks to the dispatch mechanism from NEP-18. However, it is needed, which indicates a bug in numpy wrt dispatching in np.ma. If you have the stomach for it, it would be nice to follow up with an issue upstream to sort this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's work in progress: numpy/numpy#22913

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a TODO to remove when that'll be in, and add the link to the Numpy PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too keen on that, it will take quite a while before we drop support for the last numpy version that doesn't have the feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! We can always pin numpy - but I wouldn't do that

@bouweandela bouweandela mentioned this pull request May 23, 2023
62 tasks
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking lovely, cheers, bud! Just a pointer to my only comment - that's smth we had issues with in ESMValTool antiquity times

esmvalcore/preprocessor/_regrid.py Show resolved Hide resolved
@bouweandela
Copy link
Member Author

Thanks for reviewing!

@valeriupredoi
Copy link
Contributor

My pleasure! maybe @zklaus can have a look at this one last time and merge, please 🍺

@valeriupredoi
Copy link
Contributor

@bouweandela I even fixed the conflicts, that's how charitable I feel today 😆 Maybe you can harpoon Klaus do a final review and merge?

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tacking this, @bouweandela! And thanks for the review @valeriupredoi! I've just used this branch to run a bunch of recipes, all seems fine. This PR looks good to me too, merging now 👍

@remi-kazeroni remi-kazeroni merged commit 3d6ed66 into main May 30, 2023
@remi-kazeroni remi-kazeroni deleted the lazy-extract-levels branch May 30, 2023 13:49
@bouweandela bouweandela added the dask related to improvements using Dask label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask related to improvements using Dask preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants